fix: assistant with last used and fix metadata #5955
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
![ellipsis-dev[bot]](https://avatars.githubusercontent.com/in/64358?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Important
Looks good to me! 👍
Reviewed everything up to 850fe73 in 1 minute and 12 seconds. Click for details.
- Reviewed
152lines of code in4files - Skipped
0files when reviewing. - Skipped posting
6draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. web-app/src/constants/localStorage.ts:21
- Draft comment:
Good addition of 'lastUsedAssistant' key for localStorage; centralizing keys is best practice. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
2. web-app/src/hooks/useAssistant.ts:94
- Draft comment:
Nice extension of setCurrentAssistant with the optional saveToStorage flag; consider adding a brief code comment documenting this parameter. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
3. web-app/src/hooks/useAssistant.ts:83
- Draft comment:
Deleting the current assistant properly resets to default and updates localStorage; ensure unit tests cover this fallback behavior. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
4. web-app/src/hooks/useAssistant.ts:109
- Draft comment:
initializeWithLastUsed robustly selects the persisted assistant; consider logging a debug message when the stored assistant isn’t found for easier troubleshooting. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
5. web-app/src/hooks/useChat.ts:402
- Draft comment:
Passing currentAssistant into the final content object ensures proper assistant context; verify that currentAssistant is always up-to-date. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
6. web-app/src/providers/DataProvider.tsx:40
- Draft comment:
Calling initializeWithLastUsed after setting assistants ensures the persisted selection is applied, effectively resolving #5935. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
Workflow ID: wflow_4SCMeEJt6uTmoNud
You can customize 
This was
linked to
issues
Jul 28, 2025
Contributor
Barecheck - Code coverage reportTotal: 38.6%Your code coverage diff: -0.02% ▾ Uncovered files and lines
|
Contributor
There was a problem hiding this comment.
Important
Looks good to me! 👍
Reviewed 0d004fe in 1 minute and 1 seconds. Click for details.
- Reviewed
43lines of code in1files - Skipped
0files when reviewing. - Skipped posting
4draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. web-app/src/hooks/useAssistant.ts:43
- Draft comment:
Good change: The default assistant description now uses a typographically consistent apostrophe (“user’s” vs "user's"). Ensure this change in text does not affect any downstream dependencies expecting an exact match. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
2. web-app/src/hooks/useAssistant.ts:46
- Draft comment:
Formatting consistency: The instructions string now uses curly apostrophes and improved formatting. This is purely cosmetic and improves readability. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
3. web-app/src/hooks/useAssistant.ts:89
- Draft comment:
In deleteAssistant, resetting currentAssistant and updating localStorage upon deletion of the current assistant correctly addresses the bug reported in #5935. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
4. web-app/src/hooks/useAssistant.ts:111
- Draft comment:
The initializeWithLastUsed method properly checks localStorage and falls back to the default assistant if the stored assistant isn't found. This should resolve the issue where the selected assistant resets. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
Workflow ID: wflow_Gw5TVgzv3D5uEHWX
You can customize
Contributor
There was a problem hiding this comment.
Important
Looks good to me! 👍
Reviewed 2add751 in 1 minute and 38 seconds. Click for details.
- Reviewed
115lines of code in2files - Skipped
0files when reviewing. - Skipped posting
2draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. web-app/src/hooks/useChat.ts:75
- Draft comment:
You compute aselectedAssistantfallback here, but later in the sendMessage callback you still usecurrentAssistant. For consistency (and to ensure the persisted assistant is used everywhere), consider usingselectedAssistantthroughout. - Reason this comment was not posted:
Comment was on unchanged code.
2. web-app/src/hooks/useChat.ts:245
- Draft comment:
When creating the CompletionMessagesBuilder and later in the sendCompletion call, the code still usescurrentAssistant(e.g.currentAssistant?.instructionsandcurrentAssistant.parameters). To keep behavior consistent with getCurrentThread (which uses selectedAssistant), switch these references toselectedAssistant. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
Workflow ID: wflow_T3vgfgABwjiq5LzY
You can customize
Contributor
There was a problem hiding this comment.
Important
Looks good to me! 👍
Reviewed 0ffa9eb in 1 minute and 55 seconds. Click for details.
- Reviewed
56lines of code in2files - Skipped
0files when reviewing. - Skipped posting
5draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. web-app/src/hooks/useAppState.ts:41
- Draft comment:
Ensure currentAssistant is defined before accessing its id; also check that assistants is non-empty to prevent potential runtime errors. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 20% vs. threshold = 50% While the comment identifies potential runtime issues, I need to consider: 1) Is this actually a problem? 2) The code has a fallback to assistants[0] 3) This is state management code using Zustand, which typically initializes state 4) Without seeing useAssistant's implementation, I can't be certain these checks are necessary 5) The suggestion might make the code more defensive but could also make it less readable I don't have visibility into useAssistant's implementation - these values could be guaranteed to exist. The current code might be intentionally simple because null states are handled elsewhere. Without seeing the full context of how assistants and currentAssistant are managed, suggesting additional null checks could be overly defensive and unnecessary. Delete this comment. Without strong evidence that these null states are possible or problematic, adding defensive coding would just add complexity.
2. web-app/src/hooks/useAppState.ts:41
- Draft comment:
Consider refactoring the repeated logic for selecting the assistant into a shared helper to reduce duplication. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%None
3. web-app/src/hooks/useMessages.ts:34
- Draft comment:
Ensure currentAssistant is defined and that assistants is non-empty before accessing properties to avoid potential runtime errors. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50% The code already handles the case where currentAssistant.id doesn't match any assistant by falling back to assistants[0]. TypeScript would catch undefined access. The comment is asking for verification rather than pointing out a specific issue. This seems like the kind of defensive programming that TypeScript already handles. I might be assuming too much about TypeScript's type checking. There could be edge cases where assistants is an empty array. Even if assistants is empty, this would be a configuration/initialization issue that should be caught in testing, not something to handle defensively in this hook. Delete the comment because it asks for verification rather than pointing out a specific issue, and the code already has reasonable fallbacks in place.
4. web-app/src/hooks/useMessages.ts:45
- Draft comment:
Consider adding error handling for the createMessage promise to handle potential rejections. - Reason this comment was not posted:
Comment was on unchanged code.
5. web-app/src/hooks/useMessages.ts:34
- Draft comment:
Similar to useAppState.ts, consider abstracting the assistant selection logic to avoid code duplication. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%None
Workflow ID: wflow_IHV9KArK9fnbFOTO
You can customize
Contributor
There was a problem hiding this comment.
Important
Looks good to me! 👍
Reviewed cb6b09b in 46 seconds. Click for details.
- Reviewed
14lines of code in1files - Skipped
0files when reviewing. - Skipped posting
1draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. web-app/src/hooks/__tests__/useAppState.test.ts:15
- Draft comment:
Good update – the mock now includes the 'assistants' array to reflect the new assistant persistence functionality. Ensure that this structure matches the expected schema in useAssistant. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
Workflow ID: wflow_rJA4lLZO4Qgj5bAX
You can customize
louis-jan
approved these changes
Jul 29, 2025
Member
|
LGTM! 
|
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Describe Your Changes
This pull request introduces functionality to remember and initialize the last-used assistant in the application, enhancing the user experience by persisting state across sessions. Key changes include updates to local storage management, the assistant state management logic, and the integration of this feature into the application lifecycle.
Enhancements to Assistant State Management:
getLastUsedAssistant,setLastUsedAssistant, andinitializeWithLastUsedto manage the last-used assistant in theuseAssistanthook. These methods interact with local storage to persist and retrieve the assistant state. [1] [2]deleteAssistantfunction to reset to the default assistant and update local storage if the current assistant is deleted.setCurrentAssistantfunction to optionally save the assistant to local storage.Local Storage Integration:
lastUsedAssistant, to thelocalStorageKeyconstants for storing the ID of the last-used assistant.Application Lifecycle Updates:
initializeWithLastUsedmethod into theDataProvidercomponent to automatically set the last-used assistant on application startup if available. [1] [2]Minor Adjustments:
currentAssistantto theuseChathook to ensure the correct assistant context is used during chat operations.defaultAssistantobject, replacing double quotes with single quotes for consistency.Fixes Issues
Screen.Recording.2025-07-28.at.21.35.18.mp4
Self Checklist
Important
Adds functionality to remember and initialize the last-used assistant, updating state management, local storage, and application lifecycle.
getLastUsedAssistant,setLastUsedAssistant, andinitializeWithLastUsedinuseAssistantto manage last-used assistant state.deleteAssistantto reset to default assistant if current is deleted.setCurrentAssistantto optionally save to local storage.lastUsedAssistantkey inlocalStorageKeyfor storing last-used assistant ID.initializeWithLastUsedinDataProviderto set last-used assistant on startup.currentAssistanttouseChatfor correct context.defaultAssistantobject.This description was created by
for cb6b09b. You can customize this summary. It will automatically update as commits are pushed.